Conversation
jwils
requested review from
BrianSigafoos-SQ,
ayousufi,
bsorbo,
jwondrusch and
myronmarston
as code owners
jwils
force-pushed
the
joshuaw/indexer-runtime-event-decoder
branch
from
327b53f to
cd70e31
Compare
myronmarston
left a comment
myronmarston
left a comment
There was a problem hiding this comment.
Looks good in general; left a few minor suggestions.
Can you explain how the protobuf ingestion support will build on this?
| # `#decode_events(sqs_record:, body:)` contract and return event hashes. | ||
| # | ||
| # @private | ||
| class JSONLDecoder |
There was a problem hiding this comment.
This is stateless, so we could define this as a module and do def self.decode_events below so that there's no garbage instance created for no reason.
| # @param body [String] resolved SQS message body | ||
| # @return [Array<Hash>] decoded ElasticGraph events | ||
| def decode_events(sqs_record:, body:) | ||
| _ = sqs_record |
There was a problem hiding this comment.
What's the purpose of this line? It looks like it doesn't do anything...
More generally, how do you expect sqs_record to get used?
| # @return [Array<Hash>] decoded ElasticGraph events | ||
| def decode_events(sqs_record:, body:) | ||
| _ = sqs_record | ||
| body.split("\n").map { |event| JSON.parse(event) } |
There was a problem hiding this comment.
| body.split("\n").map { |event| JSON.parse(event) } | |
| body.split("\n").map { |event| ::JSON.parse(event) } |
| def decode_events: ( | ||
| sqs_record: ::Hash[::String, untyped], | ||
| body: ::String | ||
| ) -> ::Array[::Hash[::String, untyped]] |
There was a problem hiding this comment.
| def decode_events: ( | |
| sqs_record: ::Hash[::String, untyped], | |
| body: ::String | |
| ) -> ::Array[::Hash[::String, untyped]] | |
| extend _EventPayloadDecoder |
This is the RBS equivalent of "implements the interface at the class level".
| module ElasticGraph | ||
| module IndexerLambda | ||
| class SqsProcessor | ||
| interface _EventPayloadDecoder |
There was a problem hiding this comment.
This should probably move out of SqsProcessor so it can be referenced without the SqsProcessor:: prefix. Maybe moved into the jsonl_decoder file?
| module ElasticGraph | ||
| module IndexerLambda | ||
| RSpec.describe JSONLDecoder do | ||
| describe "#decode_events" do |
There was a problem hiding this comment.
| describe "#decode_events" do | |
| describe ".decode_events" do |
(If you adopt my suggestion to make it callable directly on JSONLDecoder with no instance needed...).
2 participants
Summary
elasticgraph-indexer_lambdaSqsProcessorJSONLDecoder/SqsProcessorRBS coverage and unit coverage for happy-path and empty-body decodingWhy
main